-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add offchain clPosition descriptor #33
Conversation
/// @inheritdoc ICLPositionDescriptor | ||
function tokenURI(ICLPositionManager, uint256 tokenId) external view override returns (string memory) { | ||
return bytes(_baseTokenURI).length > 0 ? string(abi.encodePacked(_baseTokenURI, tokenId.toString())) : ""; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we do not want to do it with upgradable.
Can we add a public variable , tokenURIContract
address tokenURIContract;
if this tokenURIContract was set , not address(0) , we will read from this contract.
if it is address(0) , we will return current value.
Just a suggestion , in case we regret in the future.
|
||
/** | ||
* forge script --sig 'run(uint256)' script/01_DeployCLPositionDescriptor.s.sol:DeployCLPositionDescriptorOffChainScript <baseTokenURI> -vvv \ | ||
* --rpc-url $RPC_URL \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be --sig 'run(string)' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
script/01_DeployCLPositionDescriptor.s.sol -> script/01_DeployCLPositionDescriptorOffchain.s.sol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer if we can read from the config though, so future devs knows that params and can deploy this easily
@@ -82,6 +92,10 @@ contract CLPositionManager is | |||
_; | |||
} | |||
|
|||
function tokenURI(uint256 tokenId) public view override returns (string memory) { | |||
return ICLPositionDescriptor(tokenDescriptor).tokenURI(this, tokenId); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tokenDescriptor is already ICLPositionDescriptor type
} | ||
|
||
function setTokenURIContract(ICLPositionDescriptor newTokenURIContract) external onlyOwner { | ||
tokenURIContract = newTokenURIContract; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) others might suggest an event emitted for such cases
The PR support tokenURI() interface for CLPositionMananger. Key points:
baseTokenURI
setter (onlyOwner) in case any future update on uri rule